Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

BC-4453 - submission item file #4484

Merged
merged 32 commits into from
Nov 3, 2023
Merged

BC-4453 - submission item file #4484

merged 32 commits into from
Nov 3, 2023

Conversation

virgilchiriac
Copy link
Contributor

@virgilchiriac virgilchiriac commented Oct 18, 2023 •
edited by atlldwp
Loading

Description

  • add endpoint for creating element inside submission item
  • add file element and rich text as submission item children nodes
  • refactor board uc:
    • split column and columnBoard uc
    • introduce an abstract base.uc for common methods such as permission check
    • move some methods from "parent" uc. Such as deleteElement who is not part of element.uc instead of card.uc
Bildschirmfoto 2023-10-24 um 22 06 02

Links to Tickets or other pull requests

BC-4453
hpi-schul-cloud/nuxt-client#2893

Changes

Datasecurity

Deployment

New Repos, NPM pakages or vendor scripts

Approval for review

  • DEV: If api was changed - generate-client:server was executed in vue frontend and changes were tested and put in a PR with the same branch name.
  • QA: In addition to review, the code has been manually tested (if manual testing is possible)
  • All points were discussed with the ticket creator, support-team or product owner. The code upholds all quality guidelines from the PR-template.

Notice: Please remove the WIP label if the PR is ready to review, otherwise nobody will review it.

@virgilchiriac virgilchiriac added the WIP This feature branch is in progress, do not merge into master. label Oct 20, 2023
@virgilchiriac virgilchiriac removed the WIP This feature branch is in progress, do not merge into master. label Oct 24, 2023
@virgilchiriac virgilchiriac self-assigned this Oct 25, 2023
EzzatOmar
EzzatOmar reviewed Oct 25, 2023
View reviewed changes
Copy link
Contributor

@EzzatOmar EzzatOmar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good.
Just I think the BaseUc is not the best approach for now.
Base class make for sense for the Services (should be a different ticket IMO).

apps/server/src/modules/board/controller/api-test/content-element-create.api.spec.ts Show resolved Hide resolved
apps/server/src/modules/board/controller/api-test/content-element-create.api.spec.ts Show resolved Hide resolved
apps/server/src/modules/board/controller/dto/element/create-content-element.body.params.ts Outdated Show resolved Hide resolved
apps/server/src/modules/board/controller/dto/element/update-element-content.body.params.ts Outdated Show resolved Hide resolved
apps/server/src/modules/board/uc/base.uc.ts Show resolved Hide resolved
apps/server/src/modules/board/uc/submission-item.uc.spec.ts Outdated Show resolved Hide resolved
apps/server/src/modules/board/controller/mapper/content-element-response.factory.ts Outdated Show resolved Hide resolved
apps/server/src/modules/board/repo/recursive-save.visitor.ts Show resolved Hide resolved
apps/server/src/modules/board/uc/column.uc.spec.ts Show resolved Hide resolved
apps/server/src/modules/board/uc/element.uc.spec.ts Outdated Show resolved Hide resolved
@virgilchiriac virgilchiriac force-pushed the BC-4453-submission-item-file branch from f2a2278 to 34592eb Compare October 26, 2023 14:40
EzzatOmar
EzzatOmar requested changes Oct 27, 2023
View reviewed changes
Copy link
Contributor

@EzzatOmar EzzatOmar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would strongly advise not to include the base class and the also remove the abstraction in the visitor.

The authorization will go under major change in the next 2 sprints and it will make no sense to change any code for now.

@EzzatOmar
Copy link
Contributor

As discussed with @virgilchiriac I will approve this PR. The BaseUC class will be removed or changed in the future.
As the new role and permission system will be added.

@virgilchiriac virgilchiriac dismissed wolfganggreschus’s stale review November 3, 2023 07:29

the enpoint is designed to be as close as possible to the cards -> create element endpoint

@virgilchiriac virgilchiriac mentioned this pull request Nov 3, 2023
Merged
3 tasks
@virgilchiriac virgilchiriac enabled auto-merge (squash) November 3, 2023 10:01
@sonarqubecloud SonarQubeCloud
Copy link

sonarqubecloud bot commented Nov 3, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@virgilchiriac virgilchiriac merged commit b78c1bb into main Nov 3, 2023
45 of 46 checks passed
@virgilchiriac virgilchiriac deleted the BC-4453-submission-item-file branch November 3, 2023 11:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Metauriel Metauriel Metauriel left review comments

@EzzatOmar EzzatOmar EzzatOmar approved these changes

@wolfganggreschus wolfganggreschus Awaiting requested review from wolfganggreschus

Assignees

@virgilchiriac virgilchiriac

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@virgilchiriac @EzzatOmar @Metauriel @wolfganggreschus